-
-
Notifications
You must be signed in to change notification settings - Fork 656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return above-sea-level elevation from queryTerrainElevation + Pass lowered mercator matrix to custom layer #3854
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3854 +/- ##
==========================================
- Coverage 86.83% 86.53% -0.30%
==========================================
Files 241 241
Lines 32341 32342 +1
Branches 1962 2172 +210
==========================================
- Hits 28082 27986 -96
- Misses 3340 3408 +68
- Partials 919 948 +29 ☔ View full report in Codecov by Sentry. |
I would prefer to merge this when we work on 5.x just to be on the safe side. |
Elevation above sea level does seem more intuitive. Does our test suite validate that this is non-breaking, or was this area not covered? |
This is a breaking change, thus this PR has the 5.0 milestone. |
I am just curious, with this returning above-sea-level elevation, what happens when the elevation is below sea level? does it return a negative? |
@acalcutt , it will return an elevation from original dem data, multiplied by exxageration. Actually I didn't think about exxageration part before. |
This fixes #3736 without introducing breaking changes.
I think it would also be "the proper way" in terms of positioning custom layer objects.
Actually it creates changes to public API.
But I'll argue that they are non-breaking in a sense that all existing user-written code will work as before.
CHANGELOG.md
under the## main
section.Solution is to
queryTerrainElevation
(instead of returning this strange offset between the elevation of a given point and elevation of centerPoint).render()
method of a custom layer. "Lowered" means that mercator matrix is translated by the same Z distance as proj matrix. And this allows to use intuitive above-sea-level elevation to position custom layer objects.These 2 changes together ensure that:
Technically this PR changes the public API (queryTerrainElevation returns a different number + different matrix passed to
render
) but I believe that these 2 changes compensate each other, with all existing code working as before.That is, there is no change in how these values can be used.
So far the only sensible way to use them was to translate a given matrix by "x" (whatever is returned from queryTerrainElevation). From the user's POV, this "x" in itself was a meaningless number (some cryptic offset). And the Z component of the matrix (based on the elevation of the centerPoint) is meaningless too.
So it seems very unlikely that anyone uses them for anything other than translation of Z by x.
And this translation doesn't change, only now these values make more sense, "x" being just above-sea-level elevation. And we no longer need to explain this weird offset .
If this approach looks ok, I'll try add some tests about custom layer rendering.